-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build,windows: fix vcbuild.bat #13969
Conversation
/cc @joaocgreis @nodejs/release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added a fix to the CI script (nodejs/build#776) because the nightlies would break. There should be no issue with having both, so I believe this should land.
lgtm, thanks for getting onto it so quick @refack |
@@ -530,7 +530,7 @@ if not defined NODE_VERSION ( | |||
if not defined DISTTYPE set DISTTYPE=release | |||
if "%DISTTYPE%"=="release" ( | |||
set FULLVERSION=%NODE_VERSION% | |||
exit /b 0 | |||
goto distexit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the goto
is needed? If it's only used in this one function can't you just do:
if "%DISTTYPE%"=="release" (
set FULLVERSION=%NODE_VERSION%
+ if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%
exit /b 0
)
set FULLVERSION=%NODE_VERSION%-%TAG%
+if not defined DISTTYPEDIR set DISTTYPEDIR=%DISTTYPE%
It's possible probable I'm misunderstanding some of the subtleties of this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two implementations are functionally the same.
Here the idea is to have two parts: first do checks and set variables depending on the DISTTYPE, then goto a common block for all DISTTYPEs that may depend on the variables set above. I prefer the current change as it avoids duplicating the same set
, but no strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at some point the subroutine was used more than once... Now it's a degenerate label, but I as well would rather keep it this way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that the set repetition is also not great. If you guys prefer it this way then let's do that! I just found the goto :EOF
as the last line confusing, but hopefully long-term we can move to the Powershell promised land anyway.
* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
0c7fc82
to
9e5a211
Compare
* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit PR-URL: nodejs#13969 Refs: nodejs#13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
* rename :exit to :distexit Backport-PR-URL: #14842 PR-URL: #13969 Refs: #13900 (review) Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
restore DISTTYPEDIR
Ref: #13900 (review)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build,windows